Scope error-class throttling per-user for public-client flows - #1055
Scope error-class throttling per-user for public-client flows#1055Avery-Dunn wants to merge 3 commits into
Conversation
|
Avery-Dunn I'm not sure about this change. What happens when there is some gateway timeout? Or other 5xx messages? I mean we have to check for the user case that we have a 5xx AND a retry in the header, otherwise we could get some delays in that we dont want e.g. on 502 Bad Gateway for example. Btw. What happens when we retry? Do we consume the exception and automatically retry? |
There was a problem hiding this comment.
Pull request overview
This PR adjusts MSAL4J’s public-client request throttling so that HTTP 5xx “error-class” throttling is keyed per user, while HTTP 429 / explicit Retry-After throttling remains app-wide, preventing one user’s failures (e.g., bad password causing ADFS 500) from throttling other users sharing the same clientId/authority/scope.
Changes:
- Introduces app-wide vs user-aware throttle thumbprints and updates throttling read/write logic accordingly.
- Updates throttling checks to consult the app-wide key first, then the user-aware key when different.
- Adds regression and isolation tests for per-user 5xx throttling and app-wide 429 throttling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java |
Adds user-aware thumbprint support and routes throttling behavior based on response type (5xx vs 429/Retry-After). |
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java |
Adds regression tests ensuring 5xx throttling is per-user while 429 remains app-wide. |
| @Test | ||
| void STSResponseContains_StatusCode500_DifferentUsersNotThrottledForEachOther() throws Exception { | ||
| skipInvocationCountCheck = true; | ||
| ThrottlingCache.clear(); | ||
|
|
There was a problem hiding this comment.
Fixed in the latest commit
|
Avery-Dunn can we maybe pin this special behavior to a special status code? I want to avoid that you get DDOS-ed when some Auth Server is on bad gateway or has a error. |
"Do we consume the exception and auto-retry?": No, in most flows we use "5xx AND a Retry-After header": This was a gap in my original proposed changes, but in the latest commit it should be more consistent. The old code checked the Retry-After branch first, so a 5xx+Retry-After was throttled app-wide and it would still cause the issue in #1019. In the latest commit whether or to to throttle is now decided by status class (5xx: per-user, 429: app-wide), and Retry-After only determines the duration in either case. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java:158
- The user-aware thumbprint uses the raw UPN string as part of the hash input. UPNs are typically case-insensitive, so different casing for the same user (e.g.,
User@contoso.comvsuser@contoso.com) would produce different throttle keys and may bypass per-user throttling. Normalize the UPN (e.g., lower-case with Locale.ROOT) before hashing to keep the key stable.
// Prefer OID: it is the stable, guaranteed-unique user identifier
if (!StringHelper.isBlank(userIdentifier.oid())) {
sb.append(userIdentifier.oid()).append(POINT_DELIMITER);
} else if (!StringHelper.isBlank(userIdentifier.upn())) {
sb.append(userIdentifier.upn()).append(POINT_DELIMITER);
Fixes #1019, using the core fix made by Maximilian Pfeffer (@gterminator) in #1050 but avoiding an issue in 429 throttling scenarios, adds a couple extra tests, and allowing it to be run in our CI pipelines.
Problem
When a public-client app acquires tokens for multiple users under the same
clientId/ authority / scope, a failed request for one user could throttle every other user. Reported symptoms in #1019:MsalThrottlingException.Root cause
HttpHelperthrottles public-client requests by a "request thumbprint". Before this change the thumbprint was derived from clientId + authority + scope only — it did not include any user component for flows like Username/Password (ROPC), where there is noAccountyet. As a result two different users produced a byte-identical throttle key, so a 500 cached for user A immediately throttled user B.Fix
Make the throttle key response-type-aware:
The scope is decided by the response status class, not by the presence of a
Retry-Afterheader. An explicitRetry-Afteronly overrides the throttle duration; a 5xx that carries aRetry-Afteris still scoped per-user, and a 429 withRetry-Afteris still app-wide.checkForThrottlingnow checks the app-wide key first, then the user-aware key when it differs;processThrottlingInstructionswrites under the app-wide key for 429 and under the user-aware key for 5xx.Files changed
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.javagetRequestThumbprint(RequestContext, boolean includeUser)overload. The user component prefers OID (stable, unique) and falls back to UPN only when no OID is available (e.g. Username/Password before an account exists).checkForThrottlingchecks both app-wide and user-aware keys.processThrottlingInstructionsdecides scope by status class: 5xx → user-aware, 429 → app-wide; aRetry-Afterheader only sets the duration.Behavior change
Retry-AfterRetry-After)No public API change; no change to the cached throttle-entry value shape — only the key derivation changed. Throttle entries are transient (default window, capped by
MAX_THROTTLING_TIME_SEC).Tests
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/RequestThrottlingTest.java:UserNamePassword_DifferentUsersThrottledIndependently— user A's 500 throttles A but not user B (regression test for [Bug] Throttling cache affects whole clientId when user provides wrong password in ADFS federation #1019).UserNamePassword_429ThrottlesDifferentUsersAppWide— a 429 still throttles a different user, proving 429 stays app-wide (guards against over-narrowing).UserNamePassword_500WithRetryAfterThrottlesUsersIndependently— a 5xx carrying aRetry-Afterheader is still scoped per-user (scope is decided by status class, not by the header).STSResponseContains_StatusCode500_DifferentUsersNotThrottledForEachOtherandSilentFlow_DifferentAccountsThrottledIndependently— end-to-end and silent-flow isolation.RETRY_AFTER_HEADER,429,500, combinations) continue to pass unchanged.